Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

k8s.contrib.crd - Fix class name collision for colliding nested properties. #77

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

k8s.contrib.crd - Fix class name collision for colliding nested properties. #77

wants to merge 3 commits into from
+497 −281

Conversation

ichekrygin
Copy link

Fix colliding class names when rendering CRD with duplicated nested properties.
With this PR, the generated *pkl file changes the file naming convention for "spec.compute.field":

  • from: classes ["Spec", "Compute", "Field"]
  • to: classes ["Spec", "SpecCompute", "SpecComputeField"]

This change was suggested in #40 (comment)

Closes: #40

Currently, cards.yaml does not handle CRDs containing duplicated nested objects/lists resulting in the pkl file/module with duplicate class names. For example, if input CRD contains duplicated nested properties as in snipped below:

          properties:
            spec:
              description: Represents the configuration of a Restate Cluster
              properties:
                compute:
                  description: Compute configuration
                  properties:
                    ...
                    redObject:
                      description: Test nested objects field collision.
                      properties:
                        nestedField:
                          description: Nested field.
                          type: string
                        nestedObject:
                          description: Nested child object.
                          properties:
                            nestedRed:
                              description: Nested field.
                              type: string
                          type: object
                        nestedList:
                          description: Nested list object red.
                          items:
                            description: Red nested object test items.
                            properties:
                              nestedListItemField:
                                description: Red nested list field.
                                type: string
                            type: object
                          nullable: true
                          type: array
                      required:
                        - nestedField
                      type: object
                    blueObject:
                      description: Test nested objects field collision.
                      properties:
                        nestedField:
                          description: Nested field.
                          type: string
                        nestedObject:
                          description: Nested child object.
                          properties:
                            nestedBlue:
                              description: Nested field.
                              type: string
                          type: object
                        nestedList:
                          description: Nested list object blue.
                          items:
                            description: Blue nested object test items.
                            properties:
                              nestedListItemField:
                                description: Blue nested list field.
                                type: string
                            type: object
                          nullable: true
                          type: array
                      required:
                        - nestedField
                      type: object
                  required:
                    - image
                  type: object

We can observe colliding class definitions results in generated pkl file, such as:

...
    /// Nested child object.
    class NestedObject {
      /// Nested field.
      nestedRed: String?
    }
...
    /// Nested child object.
    class NestedObject {
      /// Nested field.
      nestedBlue: String?
    }

Similarly, with list types:

...
    /// Red nested object test items.
    class NestedList {
      /// Red nested list field.
      nestedListItemField: String?
    }
...
    /// Blue nested object test items.
    class NestedList {
      /// Blue nested list field.
      nestedListItemField: String?
    }

ichekrygin added 3 commits October 7, 2024 11:47
Enhance unit-test for packages/k8s.contrib.crd/tests/fixtures/crds.ya…
dbd5de2 
…ml adding support for colliding nested sub-objects.
Change class naming convention to avoid classes collisions on collidi…
ab5758e 
…ng properties (objects, list). This change will result in: "spec.foo.bar":

- current: classes ["Spec", "Foo", "Bar"]
- change: classes ["Spec", "SpecFoo", "SpecFooBar"

This change is suggested by https://github.com/Avarei in apple#40 (comment)
Bump major version for k8s.contrib.crd package.
9916a71
bioball
bioball requested changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@bioball bioball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this!

The issue here is really that there's a bug in the conflict detection logic; the fix is:

diff --git a/packages/k8s.contrib.crd/internal/ModuleGenerator.pkl b/packages/k8s.contrib.crd/internal/ModuleGenerator.pkl
index 4662bfb..87da6b7 100644
--- a/packages/k8s.contrib.crd/internal/ModuleGenerator.pkl
+++ b/packages/k8s.contrib.crd/internal/ModuleGenerator.pkl
@@ -281,18 +281,19 @@ function isClassLike(schema: JsonSchema.Schema): Boolean =
 /// Try to use the parent property's name as part of the class name in case of conflict.
 /// If already at the root, add a number at the end.
 local function determineTypeName(path: List<String>, candidateName: String, existingTypeNames: Set<Type>, index: Int): Type =
-  if (existingTypeNames.contains(utils.pascalCase(candidateName)))
-    if (path.isEmpty)
-      determineTypeName(path, candidateName + index.toString(), existingTypeNames, index + 1)
+  let (candidate = utils.pascalCase(candidateName))
+    if (existingTypeNames.findOrNull((it) -> it.name == candidate) != null)
+      if (path.isEmpty)
+        determineTypeName(path, candidateName + index.toString(), existingTypeNames, index + 1)
+      else
+        determineTypeName(
+          path.dropLast(1),
+          utils.pascalCase(path.last.capitalize()) + candidate,
+          existingTypeNames,
+          index
+        )
     else
-      determineTypeName(
-        path.dropLast(1),
-        utils.pascalCase(path.last.capitalize()) + utils.pascalCase(candidateName),
-        existingTypeNames,
-        index
-      )
-  else
-    new { name = utils.pascalCase(candidateName); moduleName = module.moduleName }
+      new { name = candidate; moduleName = module.moduleName }
 
 /// The schemas that should be rendered as classes.
 ///

Can you apply this diff, undo your current change? It'd be good to not incur a breaking change when we don't really need it.

@ichekrygin
Copy link
Author

ichekrygin commented Oct 10, 2024 •
edited
Loading

@bioball, (edited, after re-reading your comment).
What is the expected outcome of this diff?

@ichekrygin
Copy link
Author

With the diff, i see the following:


...
    /// Nested child object.
    class NestedObject {
      /// Nested field.
      nestedRed: String?
    }
...
    /// Nested child object.
    class NestedObjectNestedObject { <-- is that what expected?
      /// Nested field.
      nestedBlue: String?
    }

@ichekrygin
Copy link
Author

ichekrygin commented Oct 10, 2024 •
edited
Loading

I suspect the intent was:

determineTypeName(path, utils.pascalCase(path.dropLast(1).last.capitalize()) + candidate, existingTypeNames, index)

Instead of:

determineTypeName(path.dropLast(1), utils.pascalCase(path.last.capitalize()) + candidate, existingTypeNames, index)

Which results in:

 /// Nested child object.
    class BlueObjectNestedObject {
      /// Nested field.
      nestedBlue: String?
    }

@ichekrygin
Copy link
Author

@bioball , any updates on this?

@bioball
Copy link
Contributor

bioball commented Oct 23, 2024

Yeah, your suggestion looks right. Happy to accept that change!

Apologies on the late response here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bioball bioball bioball requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[K8s Contrib CRD] Duplicate class definitions in generated module
2 participants
@ichekrygin @bioball